Fix pmm deadlock #345

sparchatus · 2024-12-02T14:04:58Z

An ugly first attempt to allow pmm frame array refill without deadlocking.

Intended to fix #343

sparchatus · 2024-12-02T14:36:55Z

Optimally, we would refill the 4K frames while still holding the paging lock. However, this would need a new interface in pmm to allow refill without taking the paging lock again.

sparchatus · 2024-12-02T14:57:28Z

there is still the deadlock issue where paging wants to get a frame but pmm is waiting for paging to map a new array. It looks like some kind of backoff is required.

sparchatus · 2024-12-03T09:43:56Z

based on #344

Also fixes a (potential?) bug where total_free_frames did not count the early array frames.

jcjgraf

I am no longer able to reproduce the deadlock 👍

mm/pmm.c

wipawel · 2024-12-30T08:48:01Z

mm/pmm.c

+    spin_lock(&priority_lock);
+
+    /* if a refill is already active then they are responsible */
+    if (!refilling) {


maybe with refilling variable made atomic, this check can be done before taking the lock?

I think we could do it before and after taking the lock but I don't think it is sufficient to only check before.

Core 0: pmm_lock() -> ... -> refilling = true; unlock(priority) -> ... Core 1: refill_from_paging() -> if(!refilling) -> ... -> lock(priority)

Here paging would think there is no refilling but there is.

wipawel · 2024-12-30T08:52:59Z

mm/pmm.c

-        array = vmap_kern_4k(mfn_to_virt_map(frame->mfn), frame->mfn, L1_PROT);
+        /* switch to special refilling mode to avoid deadlock with paging */
+        ASSERT(!refilling);
+        refilling = true;


does it need to be handled under the lock below?

wipawel · 2024-12-30T08:53:50Z

mm/pmm.c

+
+        /* switch back to normal mode */
+        ASSERT(refilling);
+        refilling = false;


same question as above

mm/pmm.c

wipawel · 2024-12-30T08:59:14Z

mm/pmm.c

@@ -117,13 +149,19 @@ static frames_array_t *new_frames_array(void) {

    init_frames_array(array);

-    total_free_frames += array->meta.free_count;
+    /* since paging will back off from refilling we are responsible to do it here */
+    try_create_4k_frames();


hmm... why do we need to split frames during a frame array page mapping?

In this approach, if paging detects that refilling is already active, it will not recreate the minimum 4k frames itself but delegate it to whoever is already refilling. This avoids double refilling of array frames and also avoids inconsistencies due to two interleaved array refills.

wipawel · 2024-12-30T09:00:08Z

mm/pmm.c

    return array;
 error:
    panic("PMM: Unable to allocate new page for frame array");
    UNREACHABLE();
 }

+static frames_array_t *new_frames_array(void) {


nit: could be inline

wipawel · 2024-12-30T09:01:16Z

mm/pmm.c

+
+static void try_create_4k_frames(void);
+
+static void spin_lock_standard() {


nit: I would rename it to pmm_lock() or something like that

wipawel · 2024-12-30T09:04:57Z

arch/x86/pagetables.c

+void *vmap_frame_refill(void *va, mfn_t mfn, bool paging_lock) {
+    unsigned long _va = _ul(va) & PAGE_ORDER_TO_MASK(PAGE_ORDER_4K);
+
+    dprintk("%s: va: 0x%p mfn: 0x%lx\n", __func__, va, mfn);
+
+    if (paging_lock)
+        spin_lock(&vmap_lock);
+    else
+        ASSERT(vmap_lock);
+
+    va = _vmap_4k(&cr3, _ptr(_va), mfn, L1_PROT, false);
+
+    if (paging_lock)
+        spin_unlock(&vmap_lock);
+    else
+        ASSERT(vmap_lock);
+
+    return va;
+}
+


nit: instead of the paging_lock check I would create two versions of this function: one with locking and another with the ASSERT (or maybe a BUG_ON). Despite code duplication, it probably will pay off later when debugging something else.

wipawel · 2024-12-30T09:06:57Z

mm/pmm.c

-        array = vmap_kern_4k(mfn_to_virt_map(frame->mfn), frame->mfn, L1_PROT);
+        /* switch to special refilling mode to avoid deadlock with paging */
+        ASSERT(!refilling);
+        refilling = true;
+
+        /* only paging will be allowed to take the lock for writing while refilling */
+        spin_unlock(&priority_lock);
+        array = vmap_frame_refill(mfn_to_virt_map(frame->mfn), frame->mfn, from_paging);
+        spin_lock(&priority_lock);
+
+        /* switch back to normal mode */
+        ASSERT(refilling);
+        refilling = false;


it might make sense to handle all this code as independent functions (see my comment for the vmap_frame_refill).

wipawel · 2024-12-30T09:18:38Z

mm/pmm.c

 static size_t frames_count[MAX_PAGE_ORDER + 1];

 static spinlock_t lock = SPINLOCK_INIT;

+static bool refilling;


this could be atomic variable, maybe?

sparchatus requested a review from a team as a code owner December 2, 2024 14:04

sparchatus marked this pull request as draft December 2, 2024 15:01

sparchatus force-pushed the fix_pmm_deadlock branch from 1754bb4 to aaa42b3 Compare December 3, 2024 09:26

sparchatus marked this pull request as ready for review December 3, 2024 09:44

sparchatus changed the title ~~WIP: Fix pmm deadlock~~ Fix pmm deadlock Dec 3, 2024

sparchatus force-pushed the fix_pmm_deadlock branch 2 times, most recently from 58ae195 to 6521e20 Compare December 4, 2024 09:33

sparchatus added 7 commits December 4, 2024 10:39

cheap and ugly first attempt

e34271c

frame refill needs paging lock

dd6e8bb

allow frame refill from paging

718df32

better pmm refill implementation

808a2c2

only block relevant functions during refill

f003584

simplify and remove debug code

2285c6b

fix header

d01825b

sparchatus force-pushed the fix_pmm_deadlock branch from 6521e20 to d01825b Compare December 4, 2024 09:40

jcjgraf previously approved these changes Dec 4, 2024

View reviewed changes

wipawel self-requested a review December 4, 2024 09:51

wipawel requested changes Dec 4, 2024

View reviewed changes

mm/pmm.c Outdated Show resolved Hide resolved

pmm: add priority access for paging

60e488b

sparchatus dismissed jcjgraf’s stale review via 60e488b December 5, 2024 07:26

wipawel reviewed Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pmm deadlock #345

Fix pmm deadlock #345

sparchatus commented Dec 2, 2024

sparchatus commented Dec 2, 2024

sparchatus commented Dec 2, 2024

sparchatus commented Dec 3, 2024

jcjgraf left a comment

wipawel Dec 30, 2024

sparchatus Dec 30, 2024 •

edited

Loading

wipawel Dec 30, 2024

wipawel Dec 30, 2024

wipawel Dec 30, 2024

sparchatus Dec 30, 2024

wipawel Dec 30, 2024

wipawel Dec 30, 2024

wipawel Dec 30, 2024

wipawel Dec 30, 2024

wipawel Dec 30, 2024


		static void try_create_4k_frames(void);

		static void spin_lock_standard() {

Fix pmm deadlock #345

Are you sure you want to change the base?

Fix pmm deadlock #345

Conversation

sparchatus commented Dec 2, 2024

sparchatus commented Dec 2, 2024

sparchatus commented Dec 2, 2024

sparchatus commented Dec 3, 2024

jcjgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparchatus Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparchatus Dec 30, 2024 •

edited

Loading